Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add loading delay adminhtml #2426

Merged
merged 9 commits into from
Aug 22, 2022

Conversation

justinbeaty
Copy link
Contributor

@justinbeaty justinbeaty commented Aug 14, 2022

Description (*)

As described in #2419 it can be annoying on a fast connection that the admin dashboard "Please wait" message shows up for very fast AJAX requests.

This PR sets a timeout on showing the loading indicator so that it only shows up after 100ms. If the request is quicker than that, the indicator will not show.

I also removed some Internet Explorer stuff...

Paging @addison74 please test :)

Possible TODO, allow this value to be configurable? I'm not really sure it's needed and would cause some extra complexity because we need to output the variable value in a phtml file somewhere so that we can read it in JS. The default of 100ms is a pretty commonly used debounce value.

Related Pull Requests

Fixed Issues (if relevant)

  1. Disable page loading effect #2419

Manual testing scenarios (*)

  1. Load some quick operation in the Magento dashboard like filter customers by email foobar@example.com. The indicator should not show up if your server is fast (especially if its local)
  2. Load some longer operation in the Magento dashboard like filter customers by name Foo Bar. The indicator should show up if you have a good amount of customers.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the JavaScript Relates to js/* label Aug 14, 2022
@justinbeaty justinbeaty force-pushed the topic-adminhtml-loading-delay branch from 67326b7 to ae9e38f Compare August 14, 2022 23:23
@justinbeaty
Copy link
Contributor Author

justinbeaty commented Aug 14, 2022

I reverted the Internet Explorer stuff. It was breaking and I will create a new PR to 20.0.

@addison74
Copy link
Contributor

Thank you @justinbeaty. I can confirm that the change in this PR solves the annoying issue in the Backend interface, when even though you have a high-performance and fast server, you always receive flashes over the content of the page that it is waiting. I have sorted columns, quick switches between categories and many others, it is obviously something else. I think that the value of 100 ms is fine, at least in my case it did not appear at all.

As for making the value configurable in the interface, if it is a complex task it is not worth the effort. If in time we find that it is necessary to modify it we can make a PR that increases the value to solve that issues whether it will remain 100 ms or it will be 500 ms, we will see in time.

In any case, now I could get used to the OpenMage theme (if better colors and font), I avoided it only because these waiting were much more visually aggressive than in the Magento theme. Now it is a joy for the eyes, it's a pity that we spent so long with this issue.

@addison74
Copy link
Contributor

addison74 commented Aug 15, 2022

I made the change in production. At the value of 100 ms very few sections still display the waiting dial. If I increase the value to 150 ms the number decreases even more. It's a real relief not to see the waiting dial like a flash anymore.

We will certainly not please everyone with the value of 100 ms. If we leave it hardcoded few will know about it. Maybe we should analyze that the value is configurable in the Backend.

I would leave it at 100 ms now for testing.

@justinbeaty
Copy link
Contributor Author

I made the change in production. At the value of 100 ms very few sections still display the waiting dial. If I increase the value to 150 ms the number decreases even more. It's a real relief not to see the waiting dial like a flash anymore.

We will certainly not please everyone with the value of 100 ms. If we leave it hardcoded few will know about it. Maybe we should analyze that the value is configurable in the Backend.

I would leave it at 100 ms now for testing.

The value shouldn't be too high, but I can look into making it configurable.

One thing to test before merging - in dev environment try to set the value to something like 5000. After clicking something that would make it show up, can you break the site at all by clicking on a second thing? For example hitting a submit button twice.

If that's possible, we can work around it by making the overlay disable things on screen right away, but not visible until the timeout.

@addison74
Copy link
Contributor

addison74 commented Aug 16, 2022

In Backend > Dashboard when I click on "Most Viewed Products" tab the waiting dial appears. It is the only one I discovered for 100 ms. I will try your test and let's see what is happening.

I will look in the existing code if a configuration variable is used in any JavaScript file. If I find it, I will analyze the implementation. The value is easy to take to a PHTML file used everywhere in the Backend and from there to be taken over by the script. But maybe there is already a solution to be taken directly?

@justinbeaty
Copy link
Contributor Author

I will look in the existing code if a configuration variable is used in any JavaScript file. If I find it, I will analyze the implementation. The value is easy to take to a PHTML file used everywhere in the Backend and from there to be taken over by the script. But maybe there is already a solution to be taken directly?

There's no way to output a config variable directly into JS. You are right that there are other adminhtml variables outputted into a phtml file somewhere. I'm just not sure where off the top of my head.

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core Component: Page Relates to Mage_Page Template : admin Relates to admin template labels Aug 19, 2022
@justinbeaty
Copy link
Contributor Author

@addison74 I have added the value to the configuration dashboard. It is in: Configuration > Advanced > Admin > Admin Theme. The default is 100ms.

@justinbeaty
Copy link
Contributor Author

I am testing now to see if I can break anything by clicking other buttons before the loading mask would show up.

@addison74
Copy link
Contributor

Good job. This change really has a visual impact for all Backend users. I will test the changes and tell you my opinion. If you don't add anything and everything is fine I will ask you if I can approve it.

@addison74
Copy link
Contributor

  1. The new strings must be entered in the CSV translation file.

  2. I would mention in the comment that the default value is 100. If someone changes it even after a period forget how long it was, to know the value immediately. I am also analyzing the wording "the recommend value is 100", I have to take a look at other wordings as I want to keep the Magento format, even if some expressions are not exactly English.
    if someone sets another value and after a period is not satisfied with the results, let him know where he can take it to the end. you can mention "the recommended value is 100."

  3. We will analyze together the expressions in English because I want it not to be noticed when we add something. I am used to the interface and even if some expressions are not exactly correct they have that specific smell of Magento.

@justinbeaty
Copy link
Contributor Author

@addison74 Thanks for the reminder, I forgot about the translations.

Maybe:

Number of milliseconds to delay showing the loading indicator. The default value is "100". Enter an empty value to disable the loading indicator delay.

Adapted from #2308

@fballiano fballiano force-pushed the topic-adminhtml-loading-delay branch from d828231 to 5df1c72 Compare August 19, 2022 17:35
@justinbeaty justinbeaty force-pushed the topic-adminhtml-loading-delay branch from 585b83d to 428df90 Compare August 19, 2022 18:48
@justinbeaty
Copy link
Contributor Author

I changed how the loading mask appears. Basically I was worried about it being possible to do actions on the page when the loading indicator should have prevented any clicks (but was delayed due to this PR).

So now the <div id="loading-mask"> element appears right away, but the loading indicator child <div> is delayed. This prevents any clicks to the page immediately as before.

Despite wanting to be able to do this in pure CSS, I did have to add a new <div> to app/design/adminhtml/default/default/template/page.phtml. This only is for the new Admin theme, the legacy didn't need it. So the worst case scenario if someone is on the new theme and overrided that phtml file, that the darkened background would not show up, but that's okay IMO.

@addison74 sorry to make you test this again, but please pull the latest commits and let me know if all looks okay.

@github-actions github-actions bot added the translations Relates to app/locale label Aug 19, 2022
@addison74
Copy link
Contributor

addison74 commented Aug 19, 2022

pr2426

Comments

  1. You have chosen the right section in the configuration of the Backend (Admin > Admin Theme). I don't think it could have been anywhere else.

  2. "Loading Indicator Timeout" description is perfect.

  3. If we take into account PR Addendum to PR 2087 #2308, the last sentence should be formulated like this: "An empty value disables the loading indicator delay." or "An empty value disables the delay."

Question
What happens if there is no value in the input box? Will it be 0? I left it empty, I set the value to 0, the behavior seems to be identical, the loading indicator is no longer displayed. I looked in the code and I did not find this situation analyzed.

@justinbeaty
Copy link
Contributor Author

justinbeaty commented Aug 19, 2022

Question What happens if there is no value in the input box? Will it be 0? I left it empty, I set the value to 0, the behavior seems to be identical, the loading indicator is no longer displayed. I looked in the code and I did not find this situation analyzed.

Issue I set the value to 5 and the loading indicator is not displayed. I set it to 1, it didn't appear like that either. I doubt my system is that fast, most likely the value is not used and must be double checked. Is it possible that it is not displayed due to "display:none"?

Empty and "0" should be the same thing, a 0 millisecond timeout. Or in other words the old behavior where the indicator shows up immediately.

@addison74
Copy link
Contributor

I checked again and the changes made are correct. I appreciate that you made a validation of the value, to be greater than or equal to zero. Thank you for your work @justinbeaty.

I am waiting for you to make the change in that phrase in the comment. I would choose "An empty value disables the delay", because it is obvious that reference is made to the Loading Indicator.

@justinbeaty
Copy link
Contributor Author

@addison74 fixed translations.

addison74
addison74 previously approved these changes Aug 22, 2022
@fballiano fballiano force-pushed the topic-adminhtml-loading-delay branch from 82b5eef to 2e33abd Compare August 22, 2022 11:56
@fballiano
Copy link
Contributor

test right now, it works perfect although to me 100msec didn't do the trick, I still see the loading splash screen, with 200msec it went away (on my local dev environment on mac+homebrew)

@justinbeaty
Copy link
Contributor Author

@fballiano we can change it to 200ms, I think it makes @addison74 happier too.

@fballiano
Copy link
Contributor

I'd like that :-)

@justinbeaty
Copy link
Contributor Author

justinbeaty commented Aug 22, 2022

done @addison74 your reviews was dismissed. You can check changes here.

@fballiano fballiano merged commit 8297316 into OpenMage:1.9.4.x Aug 22, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 8297316. ± Comparison against base commit d8c52ea.

@justinbeaty justinbeaty deleted the topic-adminhtml-loading-delay branch August 23, 2022 16:08
fballiano pushed a commit that referenced this pull request Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core Component: Page Relates to Mage_Page JavaScript Relates to js/* Template : admin Relates to admin template translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants